Skip to content

Conversation

@dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jun 21, 2025

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

@dangotbanned dangotbanned marked this pull request as ready for review June 21, 2025 17:43
Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dangotbanned - should we replace all the other "unreachable" code paths as well (search)?

One nice thing that we are losing is the suggestion "Please report an issue at https://github.com/narwhals-dev/narwhals/issues/new". I wonder if it's worth it to have our version of assert_never independently of the python version

@dangotbanned
Copy link
Member Author

dangotbanned commented Jun 24, 2025

Thanks @dangotbanned

Thanks for peeking @FBruzzesi

should we replace all the other "unreachable" code paths as well (search)?

If you can find any (besides db14ab1) that both pyright and mypy agree are unreachable - I'm happy for those to use assert_never as well πŸ™‚

We can only use it when we can prove that every case has been checked, like this

image

It isn't just limited to Literal, this one is handling some pretty complex types

image

Here for example, the type hasn't been narrowed to Never - so if we used assert_never it'd produce an error during type checking

image


One nice thing that we are losing is the suggestion "Please report an issue at narwhals-dev/narwhals/issues/new". I wonder if it's worth it to have our version of assert_never independently of the python version

I'm open to using a custom message - only caveat is it has to be the same for every use.

We need to have the signature as this to be able to link whatever we use back to typing.assert_never

def assert_never(arg: Never, /) -> Never:

I was playing around with the idea of different runtime behavior than the stdlib in (#2705).
We'd basically need to do something like this here though:

from __future__ import annotations

import sys
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from typing import assert_never

    if sys.version_info >= (3, 11):
        from typing import Never
    else:
        from typing_extensions import Never

else:
    def assert_never(arg: Never, /) -> Never:
        msg = "Hello"
        raise AssertionError(msg)

@FBruzzesi
Copy link
Member

If you can find any (besides db14ab1) that both pyright and mypy agree are unreachable - I'm happy for those to use assert_never as well πŸ™‚

I will give it a shot πŸ€—

I'm open to using a custom message - only caveat is it has to be the same for every use.

We need to have the signature as this to be able to link whatever we use back to typing.assert_never

I would suggest to simply append the link at the end of the message:

def assert_never(arg: Never, /) -> Never:
    value = repr(arg)
    if len(value) > _ASSERT_NEVER_REPR_MAX_LENGTH:
        value = value[:_ASSERT_NEVER_REPR_MAX_LENGTH] + "..."
    
    msg = (
        f"Expected code to be unreachable, but got: {value}.\n"
        "Please report an issue at [narwhals-dev/narwhals/issues/new](https://github.com/narwhals-dev/narwhals/issues/new)"
    )

    raise AssertionError(msg)

dangotbanned added a commit that referenced this pull request Jun 25, 2025
- Related (#2717)

While I was looking for unreachable code, I noticed some `isinstance_or_issubclass` cases were marked as unreachable.
They *are* reachable, but when I wrote (#1997) we only had up to a **3**-tuple.
The current max of **7** wasn't covered, so this PR fixes that
Comment on lines +74 to +76
_BUG_URL = (
"https://github.com/narwhals-dev/narwhals/issues/new?template=bug_report.yml"
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes nice one! Thank you πŸ‘Œ

MarcoGorelli pushed a commit that referenced this pull request Jun 25, 2025
- Related (#2717)

While I was looking for unreachable code, I noticed some `isinstance_or_issubclass` cases were marked as unreachable.
They *are* reachable, but when I wrote (#1997) we only had up to a **3**-tuple.
The current max of **7** wasn't covered, so this PR fixes that
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks @dangotbanned !

@dangotbanned dangotbanned merged commit 0aea5a6 into main Jun 25, 2025
44 of 45 checks passed
@dangotbanned dangotbanned deleted the typing-assert-never branch June 25, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants